Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Move all metadata to pyproject.toml #1026

Open
wants to merge 9 commits into
base: develop
Choose a base branch
from

Conversation

paulf81
Copy link
Collaborator

@paulf81 paulf81 commented Nov 13, 2024

Move all metadata to pyproject.toml

Following examples such as:

https://packaging.python.org/en/latest/guides/modernize-setup-py-project/

This PR moves all metadata into pyproject.toml. Specifically:

  • All metadata now in pyproject.toml
  • setup.py and version.py deleted
  • References to setup.py and version.py updated according to guide above

@paulf81 paulf81 added the enhancement An improvement of an existing feature label Nov 13, 2024
@paulf81 paulf81 self-assigned this Nov 13, 2024
from pathlib import Path


with open(Path(__file__).parent / "version.py") as _version_file:
__version__ = _version_file.read().strip()
__version__ = version("flasc")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

😮

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed sorry for that one!

{ name = "Rafael Mudafort", email = "[email protected]" },
{ name = "Paul Fleming", email = "[email protected]" },
{ name = "Michael (Misha) Sinner", email = "[email protected]" },
{ name = "Eric Simley", email = "[email protected]" },
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably add Chris, too

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added!

python_requires=REQUIRES_PYTHON,
url=URL,
packages=find_packages(exclude=["tests", "*.tests", "*.tests.*", "tests.*"]),
package_data={
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't forget about this part

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch, thank you, think I've got it back in

@paulf81
Copy link
Collaborator Author

paulf81 commented Nov 13, 2024

@rafmudaf , 3.8 is the only failing test, but 3.8 is also end of life:
https://devguide.python.org/versions/
Should I just remove it?

@rafmudaf
Copy link
Collaborator

Changing the minimum Python version is kind of a big decision, so I wouldn't make that change just because a test is failing. However, whether to support Python 3.8 is a fair question. It may be worth asking for some community input.

@rafmudaf
Copy link
Collaborator

rafmudaf commented Nov 13, 2024

In this case, the error is because you specified Python >= 3.9, but the GitHub Actions job is testing Python 3.8. So the fix as it relates to the scope of this pull request is to use requires-python = ">=3.8".

@paulf81
Copy link
Collaborator Author

paulf81 commented Nov 13, 2024

In this case, the error is because you specified Python >= 3.9, but the GitHub Actions job is testing Python 3.8. So the fix as it relates to the scope of this pull request is to use requires-python = ">=3.8".

You're right, think I've fixed this now

@misi9170
Copy link
Collaborator

misi9170 commented Nov 14, 2024

Regarding python versions, just wanted to remind of this #953 (originally in #950 and discussed a bit more in #951)

It appears that FLORIS isn't actually currently compatible with python 3.8 and 3.9 on Windows

@paulf81
Copy link
Collaborator Author

paulf81 commented Nov 14, 2024

Nice catch @misi9170, I think I understand @rafmudaf 's point to be that raising the python requirement for FLORIS is a valid discussion but should not be done within the context of this PR as it's out of scope

@misi9170
Copy link
Collaborator

Nice catch @misi9170, I think I understand @rafmudaf 's point to be that raising the python requirement for FLORIS is a valid discussion but should not be done within the context of this PR as it's out of scope

Sounds good, agreed.

@@ -20,11 +20,11 @@ jobs:
- name: Install dependencies
run: |
python -m pip install --upgrade pip
pip install setuptools wheel twine
pip install build twine
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is the change that hung us up over on FLASC---presumably as long as thee python -m build line runs below, it should upload to PyPI correctly?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's right, I think I've gotten to learn from that and now this should go off fine, easy to test locally too but you don't want to run the twine call

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, ok, makes sense

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you want to fully test it, you could use the TestPyPI instance. If you did that, you could create a release on your fork and see that it uploads correctly without affecting anything on the primary repository.

Also heads up that this pull request is pointing to main rather than develop. Maybe that was intentional, but just pointing it out in case it wasn't.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should've been develop, moved over now

@misi9170 misi9170 self-requested a review November 14, 2024 22:10
@misi9170
Copy link
Collaborator

@paulf81 I was able to pip install this into a new conda environment and ran an example without issue.

@paulf81 paulf81 changed the base branch from main to develop November 15, 2024 16:27
@misi9170 misi9170 mentioned this pull request Nov 15, 2024
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement An improvement of an existing feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants